-
Notifications
You must be signed in to change notification settings - Fork 481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Linux ptrace plugin #1288
Add Linux ptrace plugin #1288
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally ok, bit of a bad name for a variable, but that's easy to fix. Happy with most of it, but there's parts where a list is returned by the generator rather than just multiple rows. I understand it might make the CLI output a bit worse, but for other plugins to use the output, please put each field down in its own row, rather than clumping them into a string.
architectures=LINUX_ARCHS, | ||
), | ||
requirements.PluginRequirement( | ||
name="pslist", plugin=pslist.PsList, version=(2, 2, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it important that this is 2.2.1, rather than 2.2.0? I just ask because a) I don't think our version verification checks PATCH numbers, and b) any change in API should alter the MINOR version at minimum, PATCH numbers should be transparent to the outside world. It's fine as long as it doesn't matter, I just wanted to make sure that 2.2.0 would be ok...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, yeah I know the patch number is not verified... 2.2.0 works too. Do you want me to change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can if you want for clarity/correctness, but it's not required. Just trying to avoid confusion given the PATCH number won't get checked.
flags = ( | ||
PT_FLAGS(task.ptrace).flags | ||
if task.ptrace != 0 | ||
else renderers.NotAvailableValue() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I'd prefer results being None, so that other plugins using them can easily differentiate without having to import renderers, and then just before it's output to the treegrid, converting it. Since these always seem to be NotAvailableValue
it seems like this should be pretty easy to do?
cls, | ||
context: interfaces.context.ContextInterface, | ||
symbol_table: str, | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we please get typing information on the return value, since it seems to be a generator?
else renderers.NotAvailableValue() | ||
) | ||
|
||
tracing_tids = ",".join(map(str, tracing_tid_list)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a bad idea. It's turning data into text form that then won't be possible to consume or work with if it's consumed by another plugin or a program. I'd prefer this output separate lines with one entry per line please. This might make the output quite verbose, I'm happy for you to use the Tree part of the TreeGrid to help organize these, but the point of having simple types returned is so that the data can be reused, not so it can be smushed into a human readable format and become more difficult for anything else to consume.
): | ||
formatted_field = field | ||
elif isinstance(field, objects.Array) and header_type is str: | ||
formatted_field = utility.array_to_string(field) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole method is trying to circumvent the restrictions rather than working within them. Don't pass arrays through the field, pass individual numbers with repeated data instead. There really shouldn't be any need to reformat the response from the generator before feeding it back out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The array is task.comm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sorry, I misunderstood. I thought this was for something that was a set of numbers like the tracing_tid_list
. Why wouldn't we do the conversion to string during the output, rather than yielding an array? We already know which field is going to be returned as an array, it would lose volatility information (like its layer and offset) but it's likely to be far more usable? It's also strange that knowing the field isn't an instance of header_type
, you apply it like a cast? What situations is that trying to catch?
yield level, formatted_fields | ||
|
||
def run(self): | ||
symbol_table = self.config["kernel"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config['kernel']
is the name of a module, not the name of a symbol table. Please just rename it to kernel
or kernel_module
or something similar.
def enumerate_ptraced_tasks( | ||
cls, | ||
context: interfaces.context.ContextInterface, | ||
symbol_table: str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it turns out this is a module_name
, not a symbol_table
, please name it appropriately (and ideally provide a docstring explaining what this function does).
Hey @ikelos it's now ready to go, see the example 4 with multiple rows. BTW, it seems black installation psf/black@stable is broken. EDIT: Black issue fixed in #1301 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this took me so long to re-review. It looks good now, I just have a nagging feeling that the addition of the ptrace stuff to the task struct should have a version bump somewhere, but I can't figure out exactly where? I think it'll be ok if it's known to only be part of the vol3 2.10 API though.
This PR adds the
linux.ptrace
plugin to enumerate tracer and tracee tasks. Ptrace is often leveraged by attackers to gather credentials and other sensitive information.Example 1: strace
Let's take a random process thread:
$ ps -eLf UID PID PPID LWP C NLWP STIME TTY TIME CMD root 955 1 1047 0 3 05:34 ? 00:00:00 /usr/lib/accountsservice/accounts-daemon ...
Let's ptrace the thread/task 1047
Example 2: gdb
Example 3: Ptracing from a thread
For this example, I wrote a simple C program that spawns a pthread and attaches to the same thread as described above. This shows that the information is always related to TID (and not to user PID / kernel TGID). This aligns with our discussion with @eve-mem here:
As you can see, PID in TracerPid actually refers to the user TID, representing the internal kernel PID, and not the kernel TGID which corresponds to the user PID. Confused? Please refer to the links above for clarification.
Example 4: Ptracing multiple threads using PTRACE_O_TRACECLONE
$ ps -efL | grep fork_seized ... root 1172 1146 1172 0 1 04:20 pts/0 00:00:00 sudo ./fork_seized root 1174 1172 1174 0 1 04:20 pts/0 00:00:00 ./fork_seized root 1175 1174 1175 0 4 04:20 pts/0 00:00:00 ./fork_seized root 1175 1174 1176 0 4 04:20 pts/0 00:00:00 ./fork_seized root 1175 1174 1177 0 4 04:20 pts/0 00:00:00 ./fork_seized root 1175 1174 1178 0 4 04:20 pts/0 00:00:00 ./fork_seized
$ sudo grep -H TracerPid /proc/1175/task/*/status /proc/1175/task/1175/status:TracerPid: 1174 /proc/1175/task/1176/status:TracerPid: 1174 /proc/1175/task/1177/status:TracerPid: 1174 /proc/1175/task/1178/status:TracerPid: 1174